Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CheckSchema: Verify intermediate schema upgrades #789

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

oxzi
Copy link
Member

@oxzi oxzi commented Aug 13, 2024

When skipping a version for an Icinga DB upgrade, all intermediate upgrade steps must be taken. While this is already stated in the documentation, it might be overlooked.

This happened for one community user, upgrading from v1.1.0 to v1.2.0, skipping the intermediate schema upgrade for v1.1.1.

https://community.icinga.com/t/icingadb-failing-exactly-5-minutes-after-start/13955

First, the necessity for all upgrades in their release order was made more prominent in the documentation, hoping that less users would ignore this when skimming the upgrade docs.

However, the real change here is adding another check to the CheckSchema function, verifying that all schema upgrades between the lowest known version and the highest known version in the icingadb_schema table exists. If an intermediate schema upgrade was skipped, as in the thread above, this raises a descriptive error.

@oxzi oxzi added area/documentation Improvements or additions to documentation enhancement New feature or request area/schema labels Aug 13, 2024
@oxzi oxzi requested a review from lippserd August 13, 2024 12:49
@cla-bot cla-bot bot added the cla/signed label Aug 13, 2024
pkg/icingadb/schema.go Show resolved Hide resolved
When skipping a version for an Icinga DB upgrade, all intermediate
upgrade steps must be taken. While this is already stated in the
documentation, it might be overlooked.

This happened for one community user, upgrading from v1.1.0 to v1.2.0,
skipping the intermediate schema upgrade for v1.1.1.

> https://community.icinga.com/t/icingadb-failing-exactly-5-minutes-after-start/13955

First, the necessity for all upgrades in their release order was made
more prominent in the documentation, hoping that less users would ignore
this when skimming the upgrade docs.

However, the real change here is adding another check to the CheckSchema
function, verifying that all schema upgrades between the lowest known
version and the highest known version in the icingadb_schema table
exists. If an intermediate schema upgrade was skipped, as in the thread
above, this raises a descriptive error.
@oxzi oxzi force-pushed the schema-upgrade-check-intermediates branch from 2c23f63 to 4b6ec3f Compare August 26, 2024 07:45
@oxzi oxzi requested a review from lippserd August 26, 2024 07:46
@oxzi oxzi requested a review from Al2Klimov September 6, 2024 08:44
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Al2Klimov had an alternative idea: Is it possible to add a check/throw statement at the beginning of each schema upgrade file? That way, also manual migrations would be guarded.

@oxzi
Copy link
Member Author

oxzi commented Sep 24, 2024

@Al2Klimov had an alternative idea: Is it possible to add a check/throw statement at the beginning of each schema upgrade file? That way, also manual migrations would be guarded.

While I really like this idea, I just had difficulties implementing this in a MySQL schema upgrade file in a compact, not-over-engineered way. First, MySQL's IF control structure cannot be used outside a procedure. I was unable to achieve an abortion with using the IF() function.

The shortest MySQL version I came up with looks like the following.

DROP PROCEDURE IF EXISTS schema_upgrade;

DELIMITER //
CREATE PROCEDURE schema_upgrade(expected_version SMALLINT, new_version SMALLINT)
    BEGIN
        SELECT version INTO @latest_version FROM icingadb_schema ORDER BY id DESC LIMIT 1;
        IF @latest_version != expected_version THEN
            SIGNAL SQLSTATE '45000'
                SET MESSAGE_TEXT = 'Unexpected latest schema version. Are all intermediate upgrades applied?';
        END IF;

        INSERT INTO icingadb_schema (version, timestamp) VALUES (new_version, UNIX_TIMESTAMP() * 1000);
    END //
DELIMITER ;

CALL schema_upgrade(6, 7);
DROP PROCEDURE schema_upgrade;

-- Actual schema upgrades is here.
SELECT 23;

While this still feels a bit bloated - and PostgreSQL is yet to come -, this needs to be copied to every new schema upgrade, which is a manual task we must not forget. Otherwise, the schema_upgrade procedure could be part of the regular schema, but there it would feel misplaced for me.

Don't get me wrong, I like the idea of having this logic in the database, but unless someone points me into a more practical direction, I am uncertain.

@lippserd
Copy link
Member

Otherwise, the schema_upgrade procedure could be part of the regular schema, but there it would feel misplaced for me.

Don't get me wrong, I like the idea of having this logic in the database, but unless someone points me into a more practical direction, I am uncertain.

Including the procedure directly in the baseline schema for use in future upgrades makes sense to me. But I haven't thought about when you can actually rely on it in a schema upgrade, as it needs to be introduced with a schema upgrade first. I have some ideas that we can discuss tomorrow.

@Al2Klimov
Copy link
Member

FWIW, division by zero didn't work as expected:

MariaDB [icingadb]> select 1/count(*) from icingadb_schema where version=42;
+------------+
| 1/count(*) |
+------------+
|       NULL |
+------------+
1 row in set, 1 warning (0.001 sec)

MariaDB [icingadb]>

The idea was, if it's missing, count(*) is zero, so 1/0. But I only got a warning, even with ERROR_FOR_DIVISION_BY_ZERO.

@oxzi
Copy link
Member Author

oxzi commented Sep 25, 2024

@Al2Klimov, yes, I also had to realize that the MySQL CLI continues execution regardless of errors. That's another reason why I came up with the procedure, explicitly signaling the error. This thread also contains some insights: https://stackoverflow.com/questions/773889/way-to-abort-execution-of-mysql-scripts-raising-error-perhaps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation area/schema cla/signed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants